feat(server): add OpenAPI spec generation for RESTful API#2498
Conversation
…pport Refactor REST and RPC API handlers to extract OpenAPI spec generation into dedicated classes (RestApiSpecGenerator, RPCApiSpecGenerator). Add support for queryOptions.slicing.fields with includedFilterKinds/excludedFilterKinds to control which filter operators appear in generated specs. REST generator controls individual filter query parameters; RPC generator controls per-operator properties in WhereInput schemas. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request introduces OpenAPI 3.1 specification generation for ZenStack's REST and RPC API handlers. It adds filtering utilities, type definitions, validation schemas, two new spec generator implementations, handler integrations, and comprehensive test coverage with baseline fixtures. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude finished @ymc9's task —— View job Code Review for OpenAPI Spec Generation PRI've conducted a comprehensive review of this PR that adds OpenAPI spec generation with filter kind slicing support. Here's my detailed feedback: Review Tasks:
✅ StrengthsExcellent Architecture
Robust Filter Kind Slicing Implementation
Strong Test Coverage
|
ymc9
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (4)
packages/server/test/openapi/migrated/rest-migrated.test.ts (1)
198-208: Consider usingit.skip()ortest.skipIf()for clearer test reporting.When baseline files are missing, the test returns early with a
console.warn. This passes the test silently rather than marking it as skipped in test reports. Using Vitest's skip mechanism would provide better visibility.♻️ Alternative using skipIf
it.skipIf(!fs.existsSync(path.join(__dirname, 'baseline', 'rest-3.1.0.baseline.yaml')))( 'baseline comparison - rest-3.1.0', async () => { const baselinePath = path.join(__dirname, 'baseline', 'rest-3.1.0.baseline.yaml'); const baseline = YAML.parse(fs.readFileSync(baselinePath, 'utf-8')); expect(baseline.openapi).toBe('3.1.0'); expect(baseline.components?.schemas).toBeDefined(); expect(baseline.paths).toBeDefined(); } );Alternatively, keep the current approach if you prefer tests to pass when baselines are intentionally absent during development.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/test/openapi/migrated/rest-migrated.test.ts` around lines 198 - 208, The test "baseline comparison - rest-3.1.0" currently uses an early return after checking fs.existsSync(path.join(...)) which silently passes when the baseline is missing; change this to use Vitest's skip mechanism (e.g., it.skip or it.skipIf) so the test is explicitly marked skipped instead of returning early—remove the console.warn and early return, and wrap or declare the test using it.skipIf(!fs.existsSync(path.join(__dirname, 'baseline', 'rest-3.1.0.baseline.yaml'))) while keeping the body that reads YAML.parse(fs.readFileSync(...)) and the existing expects intact.packages/server/src/api/index.ts (1)
1-3: Consider exporting the concrete spec generator classes.The
RestApiSpecGeneratorandRPCApiSpecGeneratorclasses are exported from./restand./rpcrespectively (per context snippets), but not re-exported from this main barrel module. If external consumers should have direct access to these generators (independent of the handlers), consider adding them here for API surface consistency.🔧 Optional: Add generator class exports
export { RestApiHandler, type RestApiHandlerOptions } from './rest'; export { RPCApiHandler, type RPCApiHandlerOptions } from './rpc'; export type { OpenApiSpecGenerator, OpenApiSpecOptions } from './common/types'; +export { RestApiSpecGenerator } from './rest'; +export { RPCApiSpecGenerator } from './rpc';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/api/index.ts` around lines 1 - 3, Add re-exports for the concrete spec generator classes so external consumers can import them from the main barrel; specifically export RestApiSpecGenerator and RPCApiSpecGenerator from the module by adding "export { RestApiSpecGenerator } from './rest';" and "export { RPCApiSpecGenerator } from './rpc';" alongside the existing handler and type exports (keeping the existing RestApiHandler, RPCApiHandler, and OpenApiSpecGenerator/OpenApiSpecOptions exports).packages/server/test/openapi/migrated/rpc-migrated.test.ts (2)
165-197: Same recommendation: consider usingit.skip()for missing baselines.Consistent with the REST test file feedback, using Vitest's skip mechanism would provide clearer test reporting when baseline files are absent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/test/openapi/migrated/rpc-migrated.test.ts` around lines 165 - 197, The three tests ('baseline comparison - rpc-3.1.0', 'baseline comparison - rpc-3.1.0-omit', 'baseline comparison - rpc-type-coverage-3.1.0') currently early-return with console.warn when baseline files are missing; replace that pattern with Vitest's skip so missing baselines show as skipped tests. For each test, compute baselinePath and then do: if (!fs.existsSync(baselinePath)) { it.skip('<same test title>', () => {}); return; } or better yet choose the test function dynamically (const maybeIt = fs.existsSync(baselinePath) ? it : it.skip) and call maybeIt('<same test title>', async () => { const baseline = YAML.parse(...); expect(...); }); ensuring the test name and assertions (YAML.parse and expect lines) remain unchanged.
128-129: Remove unnecessaryas anytype assertions.The
OpenAPIV3_1.InfoObjecttype from openapi-types includes bothdescriptionandsummaryas optional fields (per OpenAPI 3.1 spec). Theas anycasts on lines 128-129 are unnecessary and can be removed:expect(spec.info.description).toBe('A custom API'); expect(spec.info.summary).toBe('awesome api');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/test/openapi/migrated/rpc-migrated.test.ts` around lines 128 - 129, Remove the unnecessary `as any` casts on the OpenAPI info assertions: replace `expect((spec.info as any).description).toBe('A custom API')` and `expect((spec.info as any).summary).toBe('awesome api')` with `expect(spec.info.description).toBe('A custom API')` and `expect(spec.info.summary).toBe('awesome api')`; ensure `spec` is typed as an OpenAPIV3_1.Document (or otherwise that `spec.info` is recognized) so the `description` and `summary` properties on `spec.info` are accepted by TypeScript without casting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/server/src/api/common/spec-utils.ts`:
- Around line 24-39: The isOperationIncluded function treats a model-specific
slicing block as a full replacement of $all (using (slicing.models[modelKey] ??
slicing.models.$all)), which drops global rules; instead merge the $all block
with the model-specific block so each property (excludedOperations,
includedOperations) considers defaults from $all and overrides from
models[modelKey] before evaluating includes/excludes. Update isOperationIncluded
to compute a combined modelSlicing = { ...slicing.models.$all,
...slicing.models[modelKey] } (or equivalent property-level merge) and then
check modelSlicing.excludedOperations and modelSlicing.includedOperations; apply
the same merge-fix to the analogous logic later in the file (the field-slicing
check around lines 75-97).
In `@packages/server/src/api/rest/handler.ts`:
- Around line 1267-1298: processUpdate is manually constructing relation
payloads (set/connect) causing numeric IDs to remain strings and compound IDs to
be formatted differently than processCreate/processRelationshipCRUD; replace the
manual constructions with calls to makeIdConnect so relation payloads are
normalized. Concretely, inside the relationships loop where
updatePayload.data[key] is set for collection and non-collection branches, call
makeIdConnect(relationInfo.idFields, item.id) for each item (for collections use
set: enumerate(...).map(idObj => ({ [makeDefaultIdKey?]?: idObj }) if
makeIdConnect returns the nested unique object adjust accordingly) and for
single relations use connect: makeIdConnect(relationInfo.idFields,
data.data.id); keep updatePayload.include assignment but ensure IDs produced
match the shape used by processCreate/processRelationshipCRUD. Use the existing
symbols processUpdate, makeIdConnect, makeDefaultIdKey, processCreate,
processRelationshipCRUD, relationInfo, updatePayload and relationships to locate
and update the code.
- Around line 1817-1903: The bug is that item, curr and currType are created
once per query key and reused across enumerate(value) entries causing repeated
filter params (e.g. ?filter[id]=1&filter[id]=2) to overwrite the same object;
fix it by moving the creation/initialization of item, curr and currType inside
the for (const filterValue of enumerate(value)) loop so each filterValue builds
a fresh item object, reset curr = item and currType = typeInfo at the start of
each iteration, and then continue using existing logic (getIdFields,
getModelInfo, makeFilterValue, and pushing into items) so each value produces
its own independent filter object.
- Around line 510-512: handleGenericError currently appends err.stack into the
response body which leaks internals; change handleGenericError to only return a
generic error message via makeError('unknownError', err instanceof Error ?
err.message : 'Unknown error') and ensure the full error and stack are logged
internally (using the existing logger on the class or console.error) instead of
exposing the stack in the HTTP response; keep references to the
handleGenericError method and makeError call so the response content is
sanitized while retaining internal logs.
In `@packages/server/src/api/rest/openapi.ts`:
- Around line 73-89: generatePaths is currently adding collection and
single-resource paths unconditionally, causing operations like DELETE to be
included despite excludedOperations; update generatePaths to check
isOperationIncluded(modelName, operation) before adding each operation and when
calling buildCollectionPath and buildSinglePath so only allowed operations are
emitted—specifically, in generatePaths use isOperationIncluded for collection
ops (e.g., 'list','create') before assigning paths[`/${modelPath}`] and for
single-resource ops (e.g., 'read','update','delete') before assigning
paths[`/${modelPath}/{id}`]; also ensure the lower-level builders
(buildCollectionPath and buildSinglePath) accept or consult operation filters
(or are called selectively) so lines in the broader generation block (the later
code around 136-250) follow the same inclusion checks.
- Around line 191-250: The OpenAPI generator (buildSinglePath) is missing
operations that the runtime serves: add a PUT operation for /{model}/{id}
(operationId `replace<Model>` or `put<Model>` to match naming) with the same
requestBody schema as PATCH (`${modelName}UpdateRequest`) and appropriate
200/400/404 responses, and ensure relationship DELETE paths are generated for
/{model}/{id}/relationships/{field} (operationId like
`delete${modelName}Relationship${Field}`) returning 200/404; mirror the runtime
behavior found in RestApiHandler.handleRequest (lines handling PUT and DELETE
relationship) when creating these entries so the OpenAPI spec matches the live
endpoints.
- Around line 139-400: The responses currently reference
components/schemas/_errorResponse directly (invalid because Response Object refs
must target components/responses and Response Objects need a description);
update every occurrence inside buildCollectionPath (list/create),
buildSinglePath (get/patch/delete), buildFetchRelatedPath (get),
buildRelationshipPath (get/put/patch/post) and buildProcedureOperation to either
reference a Response Object at '#/components/responses/_errorResponse' or
replace the inline entry with a proper Response Object that includes a
description and content whose schema points to
'#/components/schemas/_errorResponse' (e.g. '400': { $ref:
'#/components/responses/_errorResponse' } or '400': { description: 'Error',
content: { 'application/vnd.api+json': { schema: { $ref:
'#/components/schemas/_errorResponse' } } } }); ensure every response entry that
previously used the schema $ref is converted accordingly.
In `@packages/server/src/api/rpc/openapi.ts`:
- Around line 73-98: The writeOps list includes ops createManyAndReturn and
updateManyAndReturn that cause references to
`${modelName}CreateManyAndReturnArgs` / `${modelName}UpdateManyAndReturnArgs`
but generateSchemas() never registers those schemas; update the generation logic
so either (A) remove those ops from writeOps or (B) add schema generation for
those args by extending opToArgsSchema/op mapping and ensure generateSchemas()
creates `${modelName}CreateManyAndReturnArgs` and
`${modelName}UpdateManyAndReturnArgs` (or maps them to existing
CreateMany/UpdateMany args) so buildPostOperation/buildPatchOperation produce
valid request-body component refs and no dangling refs. Ensure to touch
writeOps, opToArgsSchema, generateSchemas, and where
buildPostOperation/buildPatchOperation are invoked to keep names consistent.
- Around line 477-495: The recursive filter refs in the logical combinators
(properties['AND'], properties['OR'], properties['NOT']) point to
`#/components/schemas/_${_modelName}WhereInput` which doesn't match what
generateSchemas() emits (`${modelName}WhereInput`), causing dangling refs;
update those three $ref strings to reference the actual schema name produced by
generateSchemas (e.g., `#/components/schemas/${modelName}WhereInput`) or
otherwise make generateSchemas() emit the underscored name so the names match
(modify the ref strings in openapi.ts around the AND/OR/NOT blocks or adjust
generateSchemas() schema names accordingly).
- Around line 171-181: The response entries currently use a response-level $ref
to a Schema Object (e.g. { $ref: '#/components/schemas/_ErrorResponse' }) which
is invalid; update the responses in the OpenAPI generation (the responses object
built alongside modelName and op in packages/server/src/api/rpc/openapi.ts) to
either reference a Response Object (change to { $ref:
'#/components/responses/_ErrorResponse' } and add a
components.responses._ErrorResponse entry that points to the error schema) or
replace the direct $ref with a proper Response Object (include description and
content: application/json with schema: { $ref:
'#/components/schemas/_ErrorResponse' }); apply this change for all occurrences
of the error response generation so validators and generators target
`#/components/responses/` instead of `#/components/schemas/`.
- Around line 63-99: The generated OpenAPI paths always use the single-model
response schema `${modelName}Response`, causing client deserialization errors
for operations that return arrays, counts, aggregates or booleans; update the
path-building loop so each operation uses the correct response schema instead of
`${modelName}Response`. Add an op→response-schema mapping (e.g., findMany ->
`${modelName}[]` or `${modelName}ArrayResponse`,
count/createMany/updateMany/deleteMany -> `${modelName}BatchResponse` or a count
schema, aggregate -> `${modelName}AggregateResponse`, groupBy ->
`${modelName}GroupByResponse`, exists -> `${modelName}ExistsResponse`), and pass
that schema name into the builders (or extend
buildGetOperation/buildPostOperation/buildPatchOperation/buildDeleteOperation to
accept a responseSchemaName). Use the existing symbols opToArgsSchema,
buildGetOperation, buildPostOperation, buildPatchOperation,
buildDeleteOperation, isOperationIncluded and queryOptions to locate where to
inject the response schema selection and adjust the builders to reference the
provided response schema when constructing responses.
In
`@packages/server/test/openapi/migrated/baseline/rest-type-coverage-3.1.0.baseline.yaml`:
- Line 549: Fix the typo in the OpenAPI baseline description under the _links
schema: change the description string "Link for refetching the curent results"
to "Link for refetching the current results" so the word "curent" is corrected
to "current" in the description field.
- Around line 609-610: The YAML description for the schema property named
"zodErrors" has an unintended line break so the word "failure" is split onto its
own line; fix it by merging the lines into a single properly quoted or block
scalar YAML value (e.g., use a single-line string or a pipe/greater-than block)
so the description reads continuously as "Zod validation errors if the error is
due to data validation failure", and update the "description" value for the
zodErrors entry accordingly.
---
Nitpick comments:
In `@packages/server/src/api/index.ts`:
- Around line 1-3: Add re-exports for the concrete spec generator classes so
external consumers can import them from the main barrel; specifically export
RestApiSpecGenerator and RPCApiSpecGenerator from the module by adding "export {
RestApiSpecGenerator } from './rest';" and "export { RPCApiSpecGenerator } from
'./rpc';" alongside the existing handler and type exports (keeping the existing
RestApiHandler, RPCApiHandler, and OpenApiSpecGenerator/OpenApiSpecOptions
exports).
In `@packages/server/test/openapi/migrated/rest-migrated.test.ts`:
- Around line 198-208: The test "baseline comparison - rest-3.1.0" currently
uses an early return after checking fs.existsSync(path.join(...)) which silently
passes when the baseline is missing; change this to use Vitest's skip mechanism
(e.g., it.skip or it.skipIf) so the test is explicitly marked skipped instead of
returning early—remove the console.warn and early return, and wrap or declare
the test using it.skipIf(!fs.existsSync(path.join(__dirname, 'baseline',
'rest-3.1.0.baseline.yaml'))) while keeping the body that reads
YAML.parse(fs.readFileSync(...)) and the existing expects intact.
In `@packages/server/test/openapi/migrated/rpc-migrated.test.ts`:
- Around line 165-197: The three tests ('baseline comparison - rpc-3.1.0',
'baseline comparison - rpc-3.1.0-omit', 'baseline comparison -
rpc-type-coverage-3.1.0') currently early-return with console.warn when baseline
files are missing; replace that pattern with Vitest's skip so missing baselines
show as skipped tests. For each test, compute baselinePath and then do: if
(!fs.existsSync(baselinePath)) { it.skip('<same test title>', () => {}); return;
} or better yet choose the test function dynamically (const maybeIt =
fs.existsSync(baselinePath) ? it : it.skip) and call maybeIt('<same test
title>', async () => { const baseline = YAML.parse(...); expect(...); });
ensuring the test name and assertions (YAML.parse and expect lines) remain
unchanged.
- Around line 128-129: Remove the unnecessary `as any` casts on the OpenAPI info
assertions: replace `expect((spec.info as any).description).toBe('A custom
API')` and `expect((spec.info as any).summary).toBe('awesome api')` with
`expect(spec.info.description).toBe('A custom API')` and
`expect(spec.info.summary).toBe('awesome api')`; ensure `spec` is typed as an
OpenAPIV3_1.Document (or otherwise that `spec.info` is recognized) so the
`description` and `summary` properties on `spec.info` are accepted by TypeScript
without casting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f75a29d7-1077-45f0-bb75-c14f1dc714ee
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (24)
packages/server/package.jsonpackages/server/src/api/common/spec-utils.tspackages/server/src/api/common/types.tspackages/server/src/api/index.tspackages/server/src/api/rest/handler.tspackages/server/src/api/rest/index.tspackages/server/src/api/rest/openapi.tspackages/server/src/api/rpc/handler.tspackages/server/src/api/rpc/index.tspackages/server/src/api/rpc/openapi.tspackages/server/test/openapi/baseline/rest-3.1.0.baseline.yamlpackages/server/test/openapi/baseline/rest-type-coverage-3.1.0.baseline.yamlpackages/server/test/openapi/baseline/rpc-3.1.0-omit.baseline.yamlpackages/server/test/openapi/baseline/rpc-3.1.0.baseline.yamlpackages/server/test/openapi/baseline/rpc-type-coverage-3.1.0.baseline.yamlpackages/server/test/openapi/migrated/baseline/rest-3.1.0.baseline.yamlpackages/server/test/openapi/migrated/baseline/rest-type-coverage-3.1.0.baseline.yamlpackages/server/test/openapi/migrated/baseline/rpc-3.1.0-omit.baseline.yamlpackages/server/test/openapi/migrated/baseline/rpc-3.1.0.baseline.yamlpackages/server/test/openapi/migrated/baseline/rpc-type-coverage-3.1.0.baseline.yamlpackages/server/test/openapi/migrated/rest-migrated.test.tspackages/server/test/openapi/migrated/rpc-migrated.test.tspackages/server/test/openapi/rest-openapi.test.tspackages/server/test/openapi/rpc-openapi.test.ts
packages/server/test/openapi/migrated/baseline/rest-type-coverage-3.1.0.baseline.yaml
Outdated
Show resolved
Hide resolved
packages/server/test/openapi/migrated/baseline/rest-type-coverage-3.1.0.baseline.yaml
Outdated
Show resolved
Hide resolved
|
Claude finished @ymc9's task —— View job Code Review for OpenAPI Spec Generation PR
|
|
Claude finished @ymc9's task —— View job Code Review for OpenAPI Spec Generation PRI've conducted a comprehensive review of this PR that adds OpenAPI spec generation with filter kind slicing support. Here's my detailed feedback: Review Tasks:
✅ Major StrengthsExcellent Architecture & Design
Robust Filter Kind Slicing Implementation
Strong Test Coverage
|
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (3)
packages/server/src/api/rest/openapi.ts (3)
73-89:⚠️ Potential issue | 🟠 MajorHonor operation slicing before publishing REST methods.
generatePaths()always assigns both collection and single-resource path items, and the builders always return every method, soincludedOperations/excludedOperationscan't removeget,post,patch, ordeletefrom the emitted spec. Gate each method withisOperationIncluded(...)and skip empty path items.Also applies to: 136-188, 191-250
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/api/rest/openapi.ts` around lines 73 - 89, generatePaths currently unconditionally adds collection and single-resource PathItems by calling buildCollectionPath and buildSinglePath which themselves always include all methods; modify generatePaths and the builders (buildCollectionPath, buildSinglePath) to check isOperationIncluded(modelName, operation) before adding each operation (e.g., "get", "post", "patch", "delete") to the PathItem so omitted operations are not emitted, and ensure generatePaths skips adding a PathItem if it ends up empty after filtering; update logic used by getIncludedModels/getModelPath as needed so only included operations produce entries in the final PathsObject.
150-160:⚠️ Potential issue | 🔴 CriticalUse Response Objects for error statuses.
Every
400/404entry here points directly at#/components/schemas/_errorResponse. Under OpenAPI 3.1, a status-code response must be a Response Object or a Reference Object tocomponents/responses; schema refs only belong undercontent.*.schema. As written, these entries are invalid and also omit the requireddescription.#!/bin/bash set -euo pipefail file="packages/server/src/api/rest/openapi.ts" echo "Direct schema refs under response status codes:" rg -n "'(400|404)':\s*\{\s*\$ref:\s*'#/components/schemas/_errorResponse'" "$file" echo echo "Expected: no matches after wrapping these as Response Objects or moving them to components.responses."Also applies to: 175-185, 199-209, 225-236, 244-247, 278-281, 308-314, 325-328, 339-342, 360-363, 375-378
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/api/rest/openapi.ts` around lines 150 - 160, The response entries for error status codes (e.g., the '400' and '404' entries next to modelName in openapi.ts) incorrectly reference a schema (`#/components/schemas/_errorResponse`) directly instead of using a Response Object; update each such status-code entry to be either a Reference Object to components.responses (e.g., $ref: '#/components/responses/_errorResponse') or a full Response Object that includes a required description and a content block whose schema references `#/components/schemas/_errorResponse`, and apply the same change to all occurrences flagged (near the blocks that construct responses for modelNameListResponse and other endpoints). Ensure each response entry includes description when not using a $ref and replace direct schema refs under the status keys with the proper responses structure.
91-113:⚠️ Potential issue | 🟠 MajorRelation slicing still leaks endpoints and dangling refs.
generatePaths()never checksisFieldOmitted(modelName, fieldName, this.queryOptions), so omitted relation fields still get/{field}and/relationships/{field}paths. Separately,buildModelReadSchema()can still emit$ref: '#/components/schemas/${fieldDef.type}'for a kept relation whose target model was sliced out, which leaves a dangling ref because that component is never generated.Also applies to: 675-680
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/api/rest/openapi.ts` around lines 91 - 113, generatePaths() currently adds relation endpoints without checking isFieldOmitted and can refer to relation target schemas that were sliced out; update the loop over modelDef.fields in generatePaths() to skip any field where isFieldOmitted(modelName, fieldName, this.queryOptions) returns true and also ensure that before adding a relation path you verify the relation target model still exists in this.schema.models (reuse the existing relModelDef check) and that relModelDef was not sliced out; additionally, update buildModelReadSchema() to avoid emitting $ref to fieldDef.type when that target model has been omitted (check this.schema.models[fieldDef.type] or a slice/keep predicate) and emit an appropriate inline schema or remove the relation property instead to prevent dangling $ref; change references in generatePaths() to only call buildFetchRelatedPath and buildRelationshipPath when the field is not omitted and the target model is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/server/src/api/rest/openapi.ts`:
- Around line 253-284: In buildFetchRelatedPath, the 200 response currently only
has a description; update the responses object so the '200' entry includes a
proper schema $ref: use `#/components/schemas/${fieldDef.type}ListResponse` when
fieldDef.array is true (collection) and
`#/components/schemas/${fieldDef.type}Response` when false (singular). Locate
the method buildFetchRelatedPath and modify the returned get.responses['200'] to
point at the correct schema based on isCollection (using
modelName/fieldName/fieldDef to determine the type).
- Around line 671-691: buildModelReadSchema currently emits fields at the top
level which causes the later allOf merge with the _resource wrapper to put
required attributes and relation refs at the wrong level; update
buildModelReadSchema so it returns an object with type: 'object' and two
properties: attributes (an object whose properties are the non-relation scalar
fields produced via fieldToSchema, with required moved into attributes.required)
and relationships (an object whose properties are the relation fields as
ReferenceObject refs to related model schemas), keeping existing checks
isFieldOmitted, fieldDef.omit, and this.queryOptions; ensure any array/optional
logic remains but that required only lists attribute names under
attributes.required and relation fields are only referenced under relationships
rather than at the top level.
- Around line 694-757: The builders (buildCreateRequestSchema and the similar
Update builder) currently skip fields only by fieldDef.omit but don't respect
per-request field slicing; modify buildCreateRequestSchema (and the Update
counterpart) to call isFieldOmitted(_modelName, fieldName, modelDef) for each
field and skip the field when it returns true (in addition to existing checks
like fieldDef.omit/updatedAt/foreignKeyFor/id with default); ensure you actually
use the _modelName parameter passed into buildCreateRequestSchema and mirror the
same change in the update schema builder so fields removed via queryOptions are
not included in CreateRequest/UpdateRequest.
In `@packages/server/src/api/rpc/openapi.ts`:
- Around line 401-412: The loop over modelDef.fields used by the
CreateInput/UpdateInput builders calls typeToSchema(fieldDef.type) directly,
which loses nullability and array shape; update the logic in the
CreateInput/UpdateInput builders (the block that fills properties and required)
to: 1) detect when fieldDef.type represents an array and set
properties[fieldName] to an OpenAPI array schema with items produced by
typeToSchema(baseType) instead of passing the array type through unchanged; 2)
preserve nullable by setting properties[fieldName].nullable = true when the
field type allows null (or by merging/using the nullable flag returned from
typeToSchema for the inner/base type); and 3) adjust the required calculation to
include non-optional array fields (remove the current !fieldDef.array guard) so
list columns that are required remain required; ensure you reference and update
the same symbols used now (typeToSchema, modelDef.fields, properties, required)
so CreateInput and UpdateInput both get the fix.
- Around line 380-386: The generator is marking non-array, non-optional relation
fields as required which is incorrect; update the required-pushing logic in the
loop over modelDef.fields (the block using isFieldOmitted, fieldToSchema and
this.queryOptions) to skip pushing relation fields into required—i.e., add a
relation-detection check (e.g., detect relation/object-kind fields via
fieldDef.kind/type/relationship flag used in your schema representation) and
change the condition to: if (!fieldDef.optional && !fieldDef.array &&
!isRelationField(fieldDef)) { required.push(fieldName); } so relation payloads
remain optional unless explicitly selected/included.
- Around line 688-735: buildFieldFilterSchema currently uses baseSchema =
this.typeToSchema(fieldDef.type) which strips array and optional wrappers so
filters like equals/in/notIn accept only the element type and cannot represent
null or arrays; change it to build a wrapped schema that respects fieldDef.array
and fieldDef.optional (e.g. if fieldDef.array wrap baseSchema into an array
schema for equals/in/notIn where appropriate, and if fieldDef.optional mark the
schema as nullable or include null in the type) and use that wrapped schema for
filterProps entries such as 'equals', 'not', 'in', 'notIn' (while keeping
element-level schemas for list operators like 'has'/'hasEvery'/'hasSome' if
intended); update references to this.typeToSchema, fieldDef.array, and
fieldDef.optional inside buildFieldFilterSchema to produce and use
baseSchemaWithWrappers instead of the bare baseSchema.
---
Duplicate comments:
In `@packages/server/src/api/rest/openapi.ts`:
- Around line 73-89: generatePaths currently unconditionally adds collection and
single-resource PathItems by calling buildCollectionPath and buildSinglePath
which themselves always include all methods; modify generatePaths and the
builders (buildCollectionPath, buildSinglePath) to check
isOperationIncluded(modelName, operation) before adding each operation (e.g.,
"get", "post", "patch", "delete") to the PathItem so omitted operations are not
emitted, and ensure generatePaths skips adding a PathItem if it ends up empty
after filtering; update logic used by getIncludedModels/getModelPath as needed
so only included operations produce entries in the final PathsObject.
- Around line 150-160: The response entries for error status codes (e.g., the
'400' and '404' entries next to modelName in openapi.ts) incorrectly reference a
schema (`#/components/schemas/_errorResponse`) directly instead of using a
Response Object; update each such status-code entry to be either a Reference
Object to components.responses (e.g., $ref:
'#/components/responses/_errorResponse') or a full Response Object that includes
a required description and a content block whose schema references
`#/components/schemas/_errorResponse`, and apply the same change to all
occurrences flagged (near the blocks that construct responses for
modelNameListResponse and other endpoints). Ensure each response entry includes
description when not using a $ref and replace direct schema refs under the
status keys with the proper responses structure.
- Around line 91-113: generatePaths() currently adds relation endpoints without
checking isFieldOmitted and can refer to relation target schemas that were
sliced out; update the loop over modelDef.fields in generatePaths() to skip any
field where isFieldOmitted(modelName, fieldName, this.queryOptions) returns true
and also ensure that before adding a relation path you verify the relation
target model still exists in this.schema.models (reuse the existing relModelDef
check) and that relModelDef was not sliced out; additionally, update
buildModelReadSchema() to avoid emitting $ref to fieldDef.type when that target
model has been omitted (check this.schema.models[fieldDef.type] or a slice/keep
predicate) and emit an appropriate inline schema or remove the relation property
instead to prevent dangling $ref; change references in generatePaths() to only
call buildFetchRelatedPath and buildRelationshipPath when the field is not
omitted and the target model is present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d8ec89b2-c972-4703-b6c5-f4719db96185
📒 Files selected for processing (2)
packages/server/src/api/rest/openapi.tspackages/server/src/api/rpc/openapi.ts
…n OpenAPI Extract model description from @@meta("description", "...") attribute and apply it to the generated schema object in both REST and RPC OpenAPI generators. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
packages/server/src/api/rpc/openapi.ts (7)
482-497:⚠️ Potential issue | 🔴 CriticalRecursive
WhereInputrefs never resolve.
generateSchemas()only registers${modelName}WhereInput, butAND/OR/NOTpoint at#/components/schemas/_${modelName}WhereInput. Every recursive filter schema will contain dangling refs.Suggested fix
- { $ref: `#/components/schemas/_${_modelName}WhereInput` }, - { type: 'array', items: { $ref: `#/components/schemas/_${_modelName}WhereInput` } }, + { $ref: `#/components/schemas/${_modelName}WhereInput` }, + { type: 'array', items: { $ref: `#/components/schemas/${_modelName}WhereInput` } }, ... - items: { $ref: `#/components/schemas/_${_modelName}WhereInput` }, + items: { $ref: `#/components/schemas/${_modelName}WhereInput` }, ... - { $ref: `#/components/schemas/_${_modelName}WhereInput` }, - { type: 'array', items: { $ref: `#/components/schemas/_${_modelName}WhereInput` } }, + { $ref: `#/components/schemas/${_modelName}WhereInput` }, + { type: 'array', items: { $ref: `#/components/schemas/${_modelName}WhereInput` } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/api/rpc/openapi.ts` around lines 482 - 497, The recursive WhereInput refs are pointing to `#/components/schemas/_`${modelName}WhereInput while generateSchemas() registers ${modelName}WhereInput, leaving dangling refs; update the schema registration in generateSchemas() to also register the underscored name (register a schema under _${modelName}WhereInput) or change the AND/OR/NOT refs in the block that sets properties['AND'], properties['OR'], properties['NOT'] to point to `#/components/schemas/`${modelName}WhereInput so the references match—locate generateSchemas() and the properties['AND'|'OR'|'NOT'] lines to make the names consistent.
75-98:⚠️ Potential issue | 🔴 Critical
createManyAndReturnandupdateManyAndReturnreference request components that are never generated.
opToArgsSchema()resolves these ops to${modelName}CreateManyAndReturnArgsand${modelName}UpdateManyAndReturnArgs, butgenerateSchemas()never registers either component. The resulting OpenAPI will contain dangling request-body refs for both endpoints unless these ops are mapped to existing args schemas or dedicated schemas are emitted.Also applies to: 155-157, 350-354
127-130:⚠️ Potential issue | 🔴 CriticalResponse-level
$refis pointing at the wrong component type.These
400entries sit inside a Responses Object, so a$refhere has to resolve to a Response Object, not#/components/schemas/_ErrorResponse. Validators and codegen will reject the document until this is wrapped as a real response or moved undercomponents.responses.Suggested fix pattern
- '400': { $ref: '#/components/schemas/_ErrorResponse' }, + '400': { + description: 'Bad request', + content: { + 'application/json': { + schema: { $ref: '#/components/schemas/_ErrorResponse' }, + }, + }, + },In OpenAPI 3.1, when a response entry inside `responses` uses `$ref`, can it target `#/components/schemas/...`, or must it reference a Response Object under `#/components/responses/...`?Also applies to: 172-182, 204-214, 236-246, 268-278, 287-290
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/api/rpc/openapi.ts` around lines 127 - 130, The responses objects currently use a $ref that points to a schema (e.g. '400': { $ref: '#/components/schemas/_ErrorResponse' }) which is invalid because response-level $ref must resolve to a Response Object; update each offending entry (the '400' response refs and the other similar entries around the other occurrences) to either reference a Response Object under `#/components/responses` (e.g. '#/components/responses/_ErrorResponse') or replace the $ref with an inline Response Object that includes description and content referencing the schema (e.g. description plus content: application/json with schema: { $ref: '#/components/schemas/_ErrorResponse' }); ensure you apply this change for every responses block that currently points directly at the schema.
414-416:⚠️ Potential issue | 🟠 Major
CreateInputandUpdateInputdrop nullable and list field shape.These builders go straight through
typeToSchema(fieldDef.type), so nullable columns are emitted as non-null and list columns are emitted as single values. Line 415 also skips required scalar-list fields because of!fieldDef.array.Also applies to: 436-436
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/api/rpc/openapi.ts` around lines 414 - 416, CreateInput/UpdateInput builders currently call typeToSchema(fieldDef.type) directly, which loses nullable and array shapes and skips required array fields; update the property generation so that if fieldDef.array is true you wrap the element schema into an array schema (type: "array", items: typeToSchema(fieldDef.type)), and if fieldDef.nullable is true you mark the schema as nullable or allow null (e.g. set schema.nullable = true or use anyOf with {type:null}); keep the required push logic independent of fieldDef.array so non-optional array fields are included in required, and continue to respect fieldDef.default and fieldDef.optional when deciding required. Ensure these changes are applied where CreateInput/UpdateInput properties are produced and where typeToSchema is currently called.
64-99:⚠️ Potential issue | 🔴 CriticalUse operation-specific response schemas.
All CRUD builders still hardcode
${modelName}Response, whosedatafield is a single-model ref. That is wrong forfindMany,createManyAndReturn,updateManyAndReturn,count,exists,aggregate,groupBy, and the batch ops, so the emitted spec will generate incorrect client types. Plumb a response schema name per op and generate the matching response components instead of reusing the single-model wrapper everywhere.Also applies to: 172-178, 205-210, 237-242, 269-274, 672-678
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/api/rpc/openapi.ts` around lines 64 - 99, The code always uses a single `${modelName}Response` wrapper which yields incorrect types for multi-item, count, exists, aggregate, groupBy and batch-returning ops; compute an operation-specific response schema name (e.g., via a new helper like opToResponseSchema(op, modelName) or a map for ops -> schema names) and pass that responseSchemaName into the OpenAPI builders instead of relying on `${modelName}Response`. Concretely, update the loop callers (the GET loop and the writeOps loop) to compute responseSchemaName and call this.buildGetOperation(this, ...) / this.buildPostOperation / this.buildPatchOperation / this.buildDeleteOperation with the extra responseSchemaName argument, then modify those builder methods (buildGetOperation, buildPostOperation, buildPatchOperation, buildDeleteOperation) to accept and use the provided responseSchemaName to reference and emit correct response components for findMany/createManyAndReturn/updateManyAndReturn/count/exists/aggregate/groupBy and batch ops. Ensure the response component generator creates list/number/boolean/aggregate/grouped shapes as appropriate for each op.
381-386:⚠️ Potential issue | 🟠 MajorDon't require relation fields in the shared model schema.
select/includemake relation payloads conditional, but Line 385 still marks every non-optional relation as required. That makes ordinary RPC responses invalid unless the caller explicitly included the relation.Suggested fix
- if (!fieldDef.optional && !fieldDef.array) { + if (!fieldDef.optional && !fieldDef.relation) { required.push(fieldName); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/api/rpc/openapi.ts` around lines 381 - 386, The loop over modelDef.fields is currently marking every non-optional, non-array field as required; change the condition that pushes into required (in the block using fieldDef, modelDef.fields, fieldToSchema, and isFieldOmitted) to also skip relation fields so relation/select/include-driven payloads are not required; implement this by detecting relation descriptors on fieldDef (e.g., a relation flag/property such as fieldDef.relation, fieldDef.targetModel, fieldDef.isRelation, or other project-specific relation marker) and only push fieldName into required when the field is non-optional, non-array, and not a relation.
693-740:⚠️ Potential issue | 🟠 MajorFilter operators should mirror the field's value schema.
buildFieldFilterSchema()starts from baretypeToSchema(fieldDef.type), so equality/shorthand filters on list fields accept a single element and nullable fields cannot compare againstnull. Keep an element schema forhas/range-style operators, but use a wrapped value schema forequals/not/in/notInand the shorthandoneOf.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/api/rpc/openapi.ts` around lines 693 - 740, buildFieldFilterSchema currently uses baseSchema everywhere so equality/shorthand filters treat array fields as single elements and nullable fields cannot be compared to null; fix by deriving a valueSchema that represents the field's full value (if fieldDef.array then valueSchema = { type: 'array', items: baseSchema } else valueSchema = baseSchema) and if fieldDef.nullable wrap valueSchema to allow null (e.g., oneOf [valueSchema, { type: 'null' }]); then use valueSchema for equals and not, use { type: 'array', items: valueSchema } for in and notIn, keep element-based baseSchema for list operators (has/hasEvery/hasSome), and return the shorthand oneOf as { oneOf: [valueSchema, filterObject] } in buildFieldFilterSchema.
🧹 Nitpick comments (2)
packages/server/test/openapi/rest-openapi.test.ts (2)
78-79: Consider using a proper OpenAPI type forspec.Using
anyloses type safety and IDE support. If an OpenAPI type is available from a package likeopenapi-types, it would help catch assertion errors at compile time.+import type { OpenAPIV3_1 } from 'openapi-types'; + describe('REST OpenAPI spec generation', () => { let handler: RestApiHandler; - let spec: any; + let spec: OpenAPIV3_1.Document;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/test/openapi/rest-openapi.test.ts` around lines 78 - 79, The test declares spec as type any which loses type safety; change spec's type to a proper OpenAPI type (e.g., OpenAPIV3.Document from the openapi-types package) by importing the type (import type { OpenAPIV3 } from 'openapi-types') and updating the declaration (let spec: OpenAPIV3.Document) so assertions against spec in this file and usage with RestApiHandler are type-checked by TypeScript.
191-203: Consider extracting the parameter name extraction into a helper.The pattern
listOp.parameters.map((p: any) => ('name' in p ? p.name : p.$ref))is repeated 6 times throughout the file. Extracting it would improve readability and maintainability.♻️ Proposed helper function
Add near the top of the file:
const getParamNames = (params: any[]) => params.map((p: any) => ('name' in p ? p.name : p.$ref));Then replace usages:
-const paramNames = listOp.parameters.map((p: any) => ('name' in p ? p.name : p.$ref)); +const paramNames = getParamNames(listOp.parameters);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/test/openapi/rest-openapi.test.ts` around lines 191 - 203, The tests repeat the extraction logic listOp.parameters.map((p: any) => ('name' in p ? p.name : p.$ref)) so add a small helper (e.g., getParamNames) and replace direct uses with getParamNames(listOp.parameters) to improve readability; implement the helper near the top of the test file and update each occurrence (references: getParamNames, listOp.parameters) to call it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/server/src/api/rpc/openapi.ts`:
- Around line 406-410: The loops that build
CreateInput/UpdateInput/WhereUniqueInput/WhereInput currently only check
fieldDef.omit and should instead use the same visibility logic as
buildModelOutputSchema; replace checks like "if (fieldDef.omit) continue" with a
call to isFieldOmitted(modelDef, fieldName, queryOptions) (or the existing
isFieldOmitted signature used by buildModelOutputSchema) so that
queryOptions.omit/slicing are honored for request and filter schemas; update the
loops that iterate modelDef.fields (the blocks constructing CreateInput,
UpdateInput, WhereUniqueInput, WhereInput) to call isFieldOmitted before adding
a field.
---
Duplicate comments:
In `@packages/server/src/api/rpc/openapi.ts`:
- Around line 482-497: The recursive WhereInput refs are pointing to
`#/components/schemas/_`${modelName}WhereInput while generateSchemas() registers
${modelName}WhereInput, leaving dangling refs; update the schema registration in
generateSchemas() to also register the underscored name (register a schema under
_${modelName}WhereInput) or change the AND/OR/NOT refs in the block that sets
properties['AND'], properties['OR'], properties['NOT'] to point to
`#/components/schemas/`${modelName}WhereInput so the references match—locate
generateSchemas() and the properties['AND'|'OR'|'NOT'] lines to make the names
consistent.
- Around line 127-130: The responses objects currently use a $ref that points to
a schema (e.g. '400': { $ref: '#/components/schemas/_ErrorResponse' }) which is
invalid because response-level $ref must resolve to a Response Object; update
each offending entry (the '400' response refs and the other similar entries
around the other occurrences) to either reference a Response Object under
`#/components/responses` (e.g. '#/components/responses/_ErrorResponse') or replace
the $ref with an inline Response Object that includes description and content
referencing the schema (e.g. description plus content: application/json with
schema: { $ref: '#/components/schemas/_ErrorResponse' }); ensure you apply this
change for every responses block that currently points directly at the schema.
- Around line 414-416: CreateInput/UpdateInput builders currently call
typeToSchema(fieldDef.type) directly, which loses nullable and array shapes and
skips required array fields; update the property generation so that if
fieldDef.array is true you wrap the element schema into an array schema (type:
"array", items: typeToSchema(fieldDef.type)), and if fieldDef.nullable is true
you mark the schema as nullable or allow null (e.g. set schema.nullable = true
or use anyOf with {type:null}); keep the required push logic independent of
fieldDef.array so non-optional array fields are included in required, and
continue to respect fieldDef.default and fieldDef.optional when deciding
required. Ensure these changes are applied where CreateInput/UpdateInput
properties are produced and where typeToSchema is currently called.
- Around line 64-99: The code always uses a single `${modelName}Response`
wrapper which yields incorrect types for multi-item, count, exists, aggregate,
groupBy and batch-returning ops; compute an operation-specific response schema
name (e.g., via a new helper like opToResponseSchema(op, modelName) or a map for
ops -> schema names) and pass that responseSchemaName into the OpenAPI builders
instead of relying on `${modelName}Response`. Concretely, update the loop
callers (the GET loop and the writeOps loop) to compute responseSchemaName and
call this.buildGetOperation(this, ...) / this.buildPostOperation /
this.buildPatchOperation / this.buildDeleteOperation with the extra
responseSchemaName argument, then modify those builder methods
(buildGetOperation, buildPostOperation, buildPatchOperation,
buildDeleteOperation) to accept and use the provided responseSchemaName to
reference and emit correct response components for
findMany/createManyAndReturn/updateManyAndReturn/count/exists/aggregate/groupBy
and batch ops. Ensure the response component generator creates
list/number/boolean/aggregate/grouped shapes as appropriate for each op.
- Around line 381-386: The loop over modelDef.fields is currently marking every
non-optional, non-array field as required; change the condition that pushes into
required (in the block using fieldDef, modelDef.fields, fieldToSchema, and
isFieldOmitted) to also skip relation fields so relation/select/include-driven
payloads are not required; implement this by detecting relation descriptors on
fieldDef (e.g., a relation flag/property such as fieldDef.relation,
fieldDef.targetModel, fieldDef.isRelation, or other project-specific relation
marker) and only push fieldName into required when the field is non-optional,
non-array, and not a relation.
- Around line 693-740: buildFieldFilterSchema currently uses baseSchema
everywhere so equality/shorthand filters treat array fields as single elements
and nullable fields cannot be compared to null; fix by deriving a valueSchema
that represents the field's full value (if fieldDef.array then valueSchema = {
type: 'array', items: baseSchema } else valueSchema = baseSchema) and if
fieldDef.nullable wrap valueSchema to allow null (e.g., oneOf [valueSchema, {
type: 'null' }]); then use valueSchema for equals and not, use { type: 'array',
items: valueSchema } for in and notIn, keep element-based baseSchema for list
operators (has/hasEvery/hasSome), and return the shorthand oneOf as { oneOf:
[valueSchema, filterObject] } in buildFieldFilterSchema.
---
Nitpick comments:
In `@packages/server/test/openapi/rest-openapi.test.ts`:
- Around line 78-79: The test declares spec as type any which loses type safety;
change spec's type to a proper OpenAPI type (e.g., OpenAPIV3.Document from the
openapi-types package) by importing the type (import type { OpenAPIV3 } from
'openapi-types') and updating the declaration (let spec: OpenAPIV3.Document) so
assertions against spec in this file and usage with RestApiHandler are
type-checked by TypeScript.
- Around line 191-203: The tests repeat the extraction logic
listOp.parameters.map((p: any) => ('name' in p ? p.name : p.$ref)) so add a
small helper (e.g., getParamNames) and replace direct uses with
getParamNames(listOp.parameters) to improve readability; implement the helper
near the top of the test file and update each occurrence (references:
getParamNames, listOp.parameters) to call it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e241c9a7-4616-40e5-9a80-757113f92040
📒 Files selected for processing (5)
packages/server/src/api/common/spec-utils.tspackages/server/src/api/rest/openapi.tspackages/server/src/api/rpc/openapi.tspackages/server/test/openapi/rest-openapi.test.tspackages/server/test/openapi/rpc-openapi.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/server/test/openapi/rpc-openapi.test.ts
- packages/server/src/api/common/spec-utils.ts
- packages/server/src/api/rest/openapi.ts
|
Claude finished @ymc9's task —— View job Code Review for OpenAPI Spec Generation PRI've conducted a comprehensive review of this PR that adds OpenAPI spec generation with filter kind slicing support. Here's my detailed feedback: Review Tasks:
✅ Major StrengthsExcellent Architecture & Design
Robust Filter Kind Slicing Implementation
Strong Test Coverage
|
…ons validation to OpenAPI generators - REST OpenAPI generator now respects `slicing.excludedOperations`/`includedOperations` per model, mapping REST verbs to ORM operations (findMany, create, findUnique, update, delete) - Add proper zod validation schema for `queryOptions` in both REST and RPC handlers - Support `@@meta("description")` and `@meta("description")` for model/field schema descriptions - Use named arg lookup in `getMetaDescription` for reliability Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude finished @ymc9's task —— View job Code Review for OpenAPI Spec Generation PRI've conducted a comprehensive review of this PR that adds OpenAPI spec generation with filter kind slicing support. Here's my detailed feedback: Review Tasks:
✅ Major StrengthsExcellent Architecture & Design
Robust Filter Kind Slicing Implementation
Strong Test Coverage
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
packages/server/src/api/rpc/openapi.ts (7)
487-501:⚠️ Potential issue | 🔴 CriticalFix the recursive
WhereInputrefs.
AND/OR/NOTpoint at#/components/schemas/_${_modelName}WhereInput, but only${modelName}WhereInputis generated. Every recursive filter schema currently contains broken refs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/api/rpc/openapi.ts` around lines 487 - 501, The recursive filter refs for properties['AND'], properties['OR'], and properties['NOT'] currently reference `#/components/schemas/_`${_modelName}WhereInput which doesn't exist; update those references to point to the actual generated schema name (`#/components/schemas/`${_modelName}WhereInput without the extra leading underscore or to `#/components/schemas/`${modelName}WhereInput depending on how _modelName is defined) so the oneOf/array items use the real ${modelName}WhereInput schema; adjust the template where properties['AND'], properties['OR'], and properties['NOT'] are built to use the correct `${modelName}WhereInput` ref.
127-130:⚠️ Potential issue | 🔴 CriticalReference a Response Object here, not a Schema Object.
Each
400entry is$ref-ing#/components/schemas/_ErrorResponsedirectly. OpenAPI only allows a Response Object at this level, so validators and generators will reject the document until you inlinedescription/contentor move the shared error payload undercomponents.responses.In OpenAPI 3.1, can a response entry under `responses` directly use `{ "$ref": "#/components/schemas/Error" }`, or must it reference `#/components/responses/...` or inline a Response Object with `content.schema.$ref`?Also applies to: 181-181, 213-213, 245-245, 277-277, 289-289
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/api/rpc/openapi.ts` around lines 127 - 130, The OpenAPI responses are incorrectly $ref-ing a Schema Object (`#/components/schemas/_ErrorResponse`) instead of a Response Object; update each offending responses entry (the '400' response blocks at the shown spots) to either reference a Response Object under components.responses (e.g. add a components.responses.BadRequest that sets description and content.schema.$ref: '#/components/schemas/_ErrorResponse' and change the response to $ref: '#/components/responses/BadRequest') or inline a proper Response Object for '400' with description and content: {'application/json': { schema: { $ref: '#/components/schemas/_ErrorResponse' } } }; apply this change to the other listed occurrences as well.
172-177:⚠️ Potential issue | 🟠 MajorUse per-operation response components.
These builders always emit
${modelName}Response, butfindMany,count,aggregate,groupBy,exists,createMany*,updateMany*, anddeleteManydo not return a single-model payload. The generated contract will describe the wrong wire format for most RPC operations.Also applies to: 204-209, 237-242, 269-273
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/api/rpc/openapi.ts` around lines 172 - 177, The response schema always uses `${modelName}Response`, which is incorrect for many RPC ops; update the response generation in openapi.ts to choose operation-specific components instead of a single `${modelName}Response`. Implement a small helper (e.g. getResponseComponentForOp(op, modelName)) or switch on the `op` variable to return the correct schema ref for operations like findMany (e.g. `${modelName}FindManyResponse`), count (`${modelName}CountResponse`), aggregate, groupBy, exists, createMany, updateMany, deleteMany, etc., and replace the current `{ $ref: \`#/components/schemas/${modelName}Response\` }` usage with that helper so each RPC operation emits the correct response component.
78-82:⚠️ Potential issue | 🔴 CriticalGenerate
*ManyAndReturnArgscomponents before referencing them.
createManyAndReturnandupdateManyAndReturnare exposed as endpoints here, butgenerateSchemas()never registers${modelName}CreateManyAndReturnArgsor${modelName}UpdateManyAndReturnArgs. Both request-body refs are dangling right now.Also applies to: 350-354
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/api/rpc/openapi.ts` around lines 78 - 82, generateSchemas() currently doesn't register CreateManyAndReturn/UpdateManyAndReturn argument schemas yet the OpenAPI operations list in openapi.ts exposes createManyAndReturn and updateManyAndReturn endpoints, leaving ${modelName}CreateManyAndReturnArgs and ${modelName}UpdateManyAndReturnArgs refs dangling; update the schema generation logic (the function generateSchemas and/or wherever model argument schemas are built) to produce and register `${modelName}CreateManyAndReturnArgs` and `${modelName}UpdateManyAndReturnArgs` for each model (mirroring the pattern used for existing CreateManyArgs/UpdateManyArgs), ensuring the registered component names match the request-body $ref used by the operations in openapi.ts so the refs are resolved.
381-391:⚠️ Potential issue | 🟠 MajorDon't mark relation fields as required in the shared model schema.
RPC results only include relations when the caller selects/includes them. Pushing every non-optional relation into
requiredmakes ordinary responses invalid unless those relations were explicitly requested.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/api/rpc/openapi.ts` around lines 381 - 391, The loop is currently adding every non-optional, non-array field to the required array, which incorrectly includes relation fields; change the required push to skip relations by extending the condition that adds required entries—i.e., only push fieldName when (!fieldDef.optional && !fieldDef.array && !isRelation(fieldDef)). Implement isRelation detection based on the model metadata used in this codebase (e.g. check for fieldDef.relation, fieldDef.relatedModel, or fieldDef.kind === 'relation' as appropriate) so relation fields from modelDef.fields are not marked required; keep the existing calls to fieldToSchema and isFieldOmitted unchanged.
698-745:⚠️ Potential issue | 🟠 MajorMake filter values follow the field shape, not just the element type.
Starting from
typeToSchema(fieldDef.type)strips nullability and array wrappers, soequals/not/shorthand filters cannot representnullcomparisons or whole-list equality correctly. Keep the element schema for list operators likehas, but wrap the equality value schema to match the field itself.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/api/rpc/openapi.ts` around lines 698 - 745, buildFieldFilterSchema currently uses baseSchema = typeToSchema(fieldDef.type) which strips nullability/array wrappers, so equality filters (equals/not/in/notIn and the oneOf shorthand) cannot represent null or whole-list equality; compute a fullFieldSchema that preserves the field shape (nullability and array wrappers) from fieldDef (e.g., a new schema result from a variant of typeToSchema that does not unwrap arrays/nulls or reconstruct it from fieldDef), then use fullFieldSchema for equals, not, in.items, notIn.items and for the oneOf shorthand; keep using baseSchema (element schema) for list-specific operators like has/hasEvery/hasSome/isEmpty. Ensure references to buildFieldFilterSchema, typeToSchema, fieldDef, equals, not, in, notIn, has, hasEvery, hasSome, and oneOf are updated accordingly.
419-421:⚠️ Potential issue | 🟠 MajorPreserve nullable/list wrappers in create/update inputs.
typeToSchema(fieldDef.type)only returns the scalar/entity schema, so nullable scalars become non-null, scalar-list fields become scalars, and required list fields never enterrequired. ReusefieldToSchema()or an equivalent wrapper helper here.Also applies to: 441-441
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/api/rpc/openapi.ts` around lines 419 - 421, The create/update input generation uses typeToSchema(fieldDef.type) which strips nullable and array wrappers; replace those calls with fieldToSchema(fieldDef) (or a small helper that calls fieldToSchema) so the returned schema preserves nullable and list wrappers and the required array logic remains correct (so scalar-list types and nullable fields are represented properly and required only applies when appropriate); update both places where properties[fieldName] = this.typeToSchema(fieldDef.type) is used (including the other occurrence noted) to use the wrapper that accepts the full fieldDef.
🧹 Nitpick comments (1)
packages/server/test/openapi/rpc-openapi.test.ts (1)
16-24: Add a local$refsmoke test.These assertions sample a few known paths/components, but they never verify that every
#/components/schemas/*ref resolves. A tiny walker here would catch invalid-spec regressions like missing*ManyAndReturnArgscomponents or the underscoredWhereInputrefs immediately.Also applies to: 74-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/test/openapi/rpc-openapi.test.ts` around lines 16 - 24, Add a small $ref resolution smoke test that walks every schema under spec.components.schemas and asserts each local ref "#/components/schemas/..." actually resolves; implement a helper (e.g., resolveLocalRef or walkAndResolveRefs) that traverses objects, finds "$ref" strings, and for each local ref verifies spec.components.schemas[refName] is defined, then call it from the existing "document structure is valid" test (and the similar test around the 74-83 block) to fail if any component ref like "*ManyAndReturnArgs" or underscored "WhereInput" is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/server/src/api/common/schemas.ts`:
- Around line 5-27: The current validation in queryOptionsSchema (and nested
slicingSchema, modelSlicingSchema, fieldSlicingSchema) is too permissive: it
uses z.looseObject and free-form z.string().array() for operations and filter
kinds, allowing typos to pass; change z.looseObject(...) to strict z.object(...)
for queryOptionsSchema, slicingSchema, modelSlicingSchema, and
fieldSlicingSchema to reject unknown keys, and replace the free-form arrays
(e.g., includedOperations, excludedOperations, includedFilterKinds,
excludedFilterKinds) with z.enum(...) arrays that list the allowed operation
names (e.g., 'findMany', 'findUnique', 'create', 'update', 'delete', etc.) and
allowed filter kinds (e.g., 'Equality', 'Range', 'Like', 'List') so invalid
values fail validation up-front; keep the same optional() modifiers and record
shapes for models/fields as before but validate enum membership for those
arrays.
---
Duplicate comments:
In `@packages/server/src/api/rpc/openapi.ts`:
- Around line 487-501: The recursive filter refs for properties['AND'],
properties['OR'], and properties['NOT'] currently reference
`#/components/schemas/_`${_modelName}WhereInput which doesn't exist; update those
references to point to the actual generated schema name
(`#/components/schemas/`${_modelName}WhereInput without the extra leading
underscore or to `#/components/schemas/`${modelName}WhereInput depending on how
_modelName is defined) so the oneOf/array items use the real
${modelName}WhereInput schema; adjust the template where properties['AND'],
properties['OR'], and properties['NOT'] are built to use the correct
`${modelName}WhereInput` ref.
- Around line 127-130: The OpenAPI responses are incorrectly $ref-ing a Schema
Object (`#/components/schemas/_ErrorResponse`) instead of a Response Object;
update each offending responses entry (the '400' response blocks at the shown
spots) to either reference a Response Object under components.responses (e.g.
add a components.responses.BadRequest that sets description and
content.schema.$ref: '#/components/schemas/_ErrorResponse' and change the
response to $ref: '#/components/responses/BadRequest') or inline a proper
Response Object for '400' with description and content: {'application/json': {
schema: { $ref: '#/components/schemas/_ErrorResponse' } } }; apply this change
to the other listed occurrences as well.
- Around line 172-177: The response schema always uses `${modelName}Response`,
which is incorrect for many RPC ops; update the response generation in
openapi.ts to choose operation-specific components instead of a single
`${modelName}Response`. Implement a small helper (e.g.
getResponseComponentForOp(op, modelName)) or switch on the `op` variable to
return the correct schema ref for operations like findMany (e.g.
`${modelName}FindManyResponse`), count (`${modelName}CountResponse`), aggregate,
groupBy, exists, createMany, updateMany, deleteMany, etc., and replace the
current `{ $ref: \`#/components/schemas/${modelName}Response\` }` usage with
that helper so each RPC operation emits the correct response component.
- Around line 78-82: generateSchemas() currently doesn't register
CreateManyAndReturn/UpdateManyAndReturn argument schemas yet the OpenAPI
operations list in openapi.ts exposes createManyAndReturn and
updateManyAndReturn endpoints, leaving ${modelName}CreateManyAndReturnArgs and
${modelName}UpdateManyAndReturnArgs refs dangling; update the schema generation
logic (the function generateSchemas and/or wherever model argument schemas are
built) to produce and register `${modelName}CreateManyAndReturnArgs` and
`${modelName}UpdateManyAndReturnArgs` for each model (mirroring the pattern used
for existing CreateManyArgs/UpdateManyArgs), ensuring the registered component
names match the request-body $ref used by the operations in openapi.ts so the
refs are resolved.
- Around line 381-391: The loop is currently adding every non-optional,
non-array field to the required array, which incorrectly includes relation
fields; change the required push to skip relations by extending the condition
that adds required entries—i.e., only push fieldName when (!fieldDef.optional &&
!fieldDef.array && !isRelation(fieldDef)). Implement isRelation detection based
on the model metadata used in this codebase (e.g. check for fieldDef.relation,
fieldDef.relatedModel, or fieldDef.kind === 'relation' as appropriate) so
relation fields from modelDef.fields are not marked required; keep the existing
calls to fieldToSchema and isFieldOmitted unchanged.
- Around line 698-745: buildFieldFilterSchema currently uses baseSchema =
typeToSchema(fieldDef.type) which strips nullability/array wrappers, so equality
filters (equals/not/in/notIn and the oneOf shorthand) cannot represent null or
whole-list equality; compute a fullFieldSchema that preserves the field shape
(nullability and array wrappers) from fieldDef (e.g., a new schema result from a
variant of typeToSchema that does not unwrap arrays/nulls or reconstruct it from
fieldDef), then use fullFieldSchema for equals, not, in.items, notIn.items and
for the oneOf shorthand; keep using baseSchema (element schema) for
list-specific operators like has/hasEvery/hasSome/isEmpty. Ensure references to
buildFieldFilterSchema, typeToSchema, fieldDef, equals, not, in, notIn, has,
hasEvery, hasSome, and oneOf are updated accordingly.
- Around line 419-421: The create/update input generation uses
typeToSchema(fieldDef.type) which strips nullable and array wrappers; replace
those calls with fieldToSchema(fieldDef) (or a small helper that calls
fieldToSchema) so the returned schema preserves nullable and list wrappers and
the required array logic remains correct (so scalar-list types and nullable
fields are represented properly and required only applies when appropriate);
update both places where properties[fieldName] =
this.typeToSchema(fieldDef.type) is used (including the other occurrence noted)
to use the wrapper that accepts the full fieldDef.
---
Nitpick comments:
In `@packages/server/test/openapi/rpc-openapi.test.ts`:
- Around line 16-24: Add a small $ref resolution smoke test that walks every
schema under spec.components.schemas and asserts each local ref
"#/components/schemas/..." actually resolves; implement a helper (e.g.,
resolveLocalRef or walkAndResolveRefs) that traverses objects, finds "$ref"
strings, and for each local ref verifies spec.components.schemas[refName] is
defined, then call it from the existing "document structure is valid" test (and
the similar test around the 74-83 block) to fail if any component ref like
"*ManyAndReturnArgs" or underscored "WhereInput" is missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b5b09c66-02ff-48ee-8c80-d8558f369535
📒 Files selected for processing (8)
packages/server/src/api/common/schemas.tspackages/server/src/api/common/spec-utils.tspackages/server/src/api/rest/index.tspackages/server/src/api/rest/openapi.tspackages/server/src/api/rpc/index.tspackages/server/src/api/rpc/openapi.tspackages/server/test/openapi/rest-openapi.test.tspackages/server/test/openapi/rpc-openapi.test.ts
✅ Files skipped from review due to trivial changes (2)
- packages/server/test/openapi/rest-openapi.test.ts
- packages/server/src/api/rest/openapi.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/server/src/api/rpc/index.ts
- packages/server/src/api/common/spec-utils.ts
…nd remove unnecessary `as any` casts - Skip relation paths, read schema fields, and create/update relationship entries when the related model is excluded by slicing - Replace `as any` with `as OpenAPIV3_1.PathItemObject` where possible and remove unnecessary cast on `generateSharedParams()` Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude finished @ymc9's task —— View job Code Review for OpenAPI Spec Generation PRI've conducted a comprehensive review of this PR that adds OpenAPI spec generation with filter kind slicing support. Here's my detailed feedback: Review Tasks:
✅ Major StrengthsExcellent Architecture & Design
Robust Filter Kind Slicing Implementation
Strong Test Coverage
|
…and add fetch-related response schemas Error responses were using `$ref` to schemas directly in response positions, which is invalid per OpenAPI spec. Replace with inline Response Objects containing description and content. Also add proper response schema references to fetch-related endpoints. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude finished @ymc9's task —— View job Code Review for OpenAPI Spec Generation PRI've conducted a comprehensive review of this PR that adds OpenAPI spec generation with filter kind slicing support. Here's my detailed feedback: Review Tasks:
✅ Major StrengthsExcellent Architecture & Design
Robust Filter Kind Slicing Implementation
Strong Test Coverage
|
|
Claude finished @ymc9's task —— View job Code Review for OpenAPI Spec Generation PRI've conducted a comprehensive review of this PR that adds OpenAPI spec generation with filter kind slicing support. Here's my detailed feedback: Review Tasks:
✅ Major StrengthsExcellent Architecture & Design
Robust Filter Kind Slicing Implementation
Strong Test Coverage
|
|
Claude finished @ymc9's task —— View job Code Review for OpenAPI Spec Generation PRI've conducted a comprehensive review of this PR that adds OpenAPI spec generation with filter kind slicing support. Here's my detailed feedback: Review Tasks:
✅ Major StrengthsExcellent Architecture & Design
Robust Filter Kind Slicing Implementation
Strong Test Coverage
|
Merge migrated baseline tests into main test files, add @readme/openapi-parser validation, and regenerate baselines with 4-space indent. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude finished @ymc9's task —— View job Code Review for OpenAPI Spec Generation PRI've conducted a comprehensive review of this PR that adds OpenAPI spec generation with filter kind slicing support. Here's my detailed feedback: Review Tasks:
✅ Major StrengthsExcellent Architecture & Design
Robust Filter Kind Slicing Implementation
Strong Test Coverage
|
Unify filter schema creation into a single `buildFilterSchema` method with optional slicing context to eliminate duplication. Shared filter schemas (_StringFilter, _IntFilter, etc.) are now generated once and referenced via $ref. Also adds update-baseline npm script and regenerates baselines. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude finished @ymc9's task —— View job Code Review for OpenAPI Spec Generation PRI've conducted a comprehensive review of this PR that adds OpenAPI spec generation with filter kind slicing support. Here's my detailed feedback: Review Tasks:
✅ Major StrengthsExcellent Architecture & Design
Robust Filter Kind Slicing Implementation
Strong Test Coverage
|
|
Claude finished @ymc9's task —— View job Code Review for OpenAPI Spec Generation PR
|
ymc9
changed the title
|
Claude finished @ymc9's task —— View job Code Review for OpenAPI Spec Generation PRI've conducted a comprehensive review of this PR that adds OpenAPI spec generation with filter kind slicing support. Here's my detailed feedback: Review Tasks:
✅ Major StrengthsExcellent Architecture & Design
Robust Filter Kind Slicing Implementation
Strong Test Coverage
|
Restructure buildModelReadSchema to nest scalar fields under `attributes` and relation fields under `relationships`, matching JSON:API resource structure. Relation fields now use proper relationship refs (_toManyRelationshipWithLinks/_toOneRelationshipWithLinks). Also generate schemas for composite type definitions (typeDefs). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude finished @ymc9's task —— View job Code Review for OpenAPI Spec Generation PR
|
|
@claude redo a full thorough review |
|
Claude finished @ymc9's task —— View job Code Review for OpenAPI Spec Generation PRI've conducted a comprehensive review of this PR that adds OpenAPI spec generation with filter kind slicing support. Here's my detailed feedback: Review Tasks:
✅ Major StrengthsExcellent Architecture & Design
Robust Filter Kind Slicing Implementation
Strong Test Coverage
|
1 participant
Summary
RestApiSpecGeneratorandRPCApiSpecGeneratorclassesspec-utils.tswith helpers for model/operation/field/filter-kind inclusion checksqueryOptions.slicing.fieldswithincludedFilterKinds/excludedFilterKindsto control which filter operators appear in generated OpenAPI specsfilter[field][op]query parameters per filter kind (Equality, Like, Range, List)equals,contains,lt, etc.) inWhereInputschemas, withoneOfshorthand when Equality is includedqueryOptionssupport to handler constructors (with Zod validation) foromitandslicingconfigurationTest plan
npx vitest run packages/server/test/openapi/)excludedFilterKinds/includedFilterKindsWhereInputschemas reflect per-operator filter kind control$allfield-level slicing applies to all fields with field-specific overrides🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores